refactor: 容器注解补 Any 参数 + 移除 reportMissingTypeArgument 全局抑制 - #3
Conversation
Reviewer's Guide本次 PR 在保持 pyright 严格模式(strict mode)的前提下,通过引入共享的 JSON/MeasureState 类型别名、添加运行时类型守卫/类型转换,以及清理回调和 MaaFramework 相关类型标注,收紧整个应用中的 JSON 和动态状态类型,从而让严格模式下的 pyright 在不依赖大范围 suppress 的情况下顺利通过。 File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your Experience访问你的 dashboard 来:
Getting HelpOriginal review guide in EnglishReviewer's GuideThis PR keeps pyright in strict mode while tightening JSON and dynamic-state typing across the app by introducing shared JSON/MeasureState aliases, adding runtime type guards/casts, and cleaning up callback and MaaFramework-related typing so strict pyright passes without broad suppressions. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - 我发现了两个问题,并留下了一些整体性的反馈:
- 在
floating_state.py和window_snap.py中引入的临时_to_int辅助函数是相同的;可以考虑把这类逻辑(以及类似的 JSON 数值规范化逻辑)集中到一个共享工具中,以避免重复并保持行为一致。 - 既然已经引入了
JsonObject和MeasureState,你可能想再扫描一下剩余的动态 JSONdict[str, Any]用法,并将它们统一替换成这些别名,使类型边界更加清晰和统一。 - 在
detect_negative_cost中,额外的white_rows类型转换和基于索引的循环增加了复杂度;你可以直接遍历white(配合显式的np.ndarray注解),在满足严格类型检查的同时让代码更简单一些。
面向 AI Agent 的提示
Please address the comments from this code review:
## Overall Comments
- The ad-hoc `_to_int` helpers introduced in `floating_state.py` and `window_snap.py` are identical; consider centralizing this logic (and similar JSON numeric normalization) in a shared utility to avoid duplication and keep behavior consistent.
- Now that `JsonObject` and `MeasureState` are introduced, you might want to scan for remaining dynamic-JSON `dict[str, Any]` usages and align them with these aliases to make the typing boundary even clearer and more uniform.
- In `detect_negative_cost` the extra `white_rows` cast and index-based loop add complexity; you could iterate directly over `white` (with an explicit `np.ndarray` annotation) to keep the code simpler while still satisfying strict type checking.
## Individual Comments
### Comment 1
<location path="aao/ui/floating_state.py" line_range="61-62" />
<code_context>
if not isinstance(g, list | tuple):
return
- x, y, w, h = [int(v) for v in g]
+ values = [_to_int(v) for v in cast(list[object] | tuple[object, ...], g)]
+ x, y, w, h = values
widget.setGeometry(x, y, w, h)
</code_context>
<issue_to_address>
**issue (bug_risk):** Restore geometry should guard against sequences with length != 4 to avoid runtime errors.
`restore_geometry` only checks that `g` is a list/tuple, not that it has length 4. If stored geometry has a different length (e.g., corrupted settings), `x, y, w, h = values` will raise `ValueError`. Please reuse `_valid_geometry` (or at least its length check) before unpacking so invalid data is skipped instead of crashing.
</issue_to_address>
### Comment 2
<location path="aao/resources/updater.py" line_range="98-99" />
<code_context>
req.add_header("Authorization", f"Bearer {token}")
with urllib.request.urlopen(req, timeout=10) as resp:
- data = json.loads(resp.read())
+ raw = json.loads(resp.read())
+ data = cast(JsonObject, raw) if isinstance(raw, dict) else {}
except Exception as e: # noqa: BLE001
logger.warning("检查更新失败: %s", e)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Unexpected response shapes from GitHub are silently treated as empty dict, which can hide issues.
When GitHub returns a non-dict payload (e.g., error JSON or HTML), `raw` is discarded and `data` becomes `{}` with no extra logging, so callers just see “no update” instead of a failed check. Consider logging a warning when `raw` is not a dict (or returning `None`) so the failure is visible to callers.
Suggested implementation:
```python
if token:
req.add_header("Authorization", f"Bearer {token}")
with urllib.request.urlopen(req, timeout=10) as resp:
raw = json.loads(resp.read())
if isinstance(raw, dict):
data = cast(JsonObject, raw)
else:
logger.warning("检查更新失败,返回 payload 非 JSON 对象: %r", raw)
return None
except Exception as e: # noqa: BLE001
logger.warning("检查更新失败: %s", e)
return None
```
```python
try:
req = _make_request(_RELEASES_LIST_API, _settings_github_token())
with urllib.request.urlopen(req, timeout=10) as resp:
raw = json.loads(resp.read())
if isinstance(raw, list):
releases = cast(list[JsonObject], raw)
else:
logger.warning("拉取 release 列表失败,返回 payload 非 JSON 数组: %r", raw)
return ""
except Exception as e: # noqa: BLE001
logger.warning("拉取 release 列表失败,降级为单版 changelog: %s", e)
return ""
```帮我变得更有用!请在每条评论上点 👍 或 👎,我会根据你的反馈改进评审质量。
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
- The ad-hoc
_to_inthelpers introduced infloating_state.pyandwindow_snap.pyare identical; consider centralizing this logic (and similar JSON numeric normalization) in a shared utility to avoid duplication and keep behavior consistent. - Now that
JsonObjectandMeasureStateare introduced, you might want to scan for remaining dynamic-JSONdict[str, Any]usages and align them with these aliases to make the typing boundary even clearer and more uniform. - In
detect_negative_costthe extrawhite_rowscast and index-based loop add complexity; you could iterate directly overwhite(with an explicitnp.ndarrayannotation) to keep the code simpler while still satisfying strict type checking.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The ad-hoc `_to_int` helpers introduced in `floating_state.py` and `window_snap.py` are identical; consider centralizing this logic (and similar JSON numeric normalization) in a shared utility to avoid duplication and keep behavior consistent.
- Now that `JsonObject` and `MeasureState` are introduced, you might want to scan for remaining dynamic-JSON `dict[str, Any]` usages and align them with these aliases to make the typing boundary even clearer and more uniform.
- In `detect_negative_cost` the extra `white_rows` cast and index-based loop add complexity; you could iterate directly over `white` (with an explicit `np.ndarray` annotation) to keep the code simpler while still satisfying strict type checking.
## Individual Comments
### Comment 1
<location path="aao/ui/floating_state.py" line_range="61-62" />
<code_context>
if not isinstance(g, list | tuple):
return
- x, y, w, h = [int(v) for v in g]
+ values = [_to_int(v) for v in cast(list[object] | tuple[object, ...], g)]
+ x, y, w, h = values
widget.setGeometry(x, y, w, h)
</code_context>
<issue_to_address>
**issue (bug_risk):** Restore geometry should guard against sequences with length != 4 to avoid runtime errors.
`restore_geometry` only checks that `g` is a list/tuple, not that it has length 4. If stored geometry has a different length (e.g., corrupted settings), `x, y, w, h = values` will raise `ValueError`. Please reuse `_valid_geometry` (or at least its length check) before unpacking so invalid data is skipped instead of crashing.
</issue_to_address>
### Comment 2
<location path="aao/resources/updater.py" line_range="98-99" />
<code_context>
req.add_header("Authorization", f"Bearer {token}")
with urllib.request.urlopen(req, timeout=10) as resp:
- data = json.loads(resp.read())
+ raw = json.loads(resp.read())
+ data = cast(JsonObject, raw) if isinstance(raw, dict) else {}
except Exception as e: # noqa: BLE001
logger.warning("检查更新失败: %s", e)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Unexpected response shapes from GitHub are silently treated as empty dict, which can hide issues.
When GitHub returns a non-dict payload (e.g., error JSON or HTML), `raw` is discarded and `data` becomes `{}` with no extra logging, so callers just see “no update” instead of a failed check. Consider logging a warning when `raw` is not a dict (or returning `None`) so the failure is visible to callers.
Suggested implementation:
```python
if token:
req.add_header("Authorization", f"Bearer {token}")
with urllib.request.urlopen(req, timeout=10) as resp:
raw = json.loads(resp.read())
if isinstance(raw, dict):
data = cast(JsonObject, raw)
else:
logger.warning("检查更新失败,返回 payload 非 JSON 对象: %r", raw)
return None
except Exception as e: # noqa: BLE001
logger.warning("检查更新失败: %s", e)
return None
```
```python
try:
req = _make_request(_RELEASES_LIST_API, _settings_github_token())
with urllib.request.urlopen(req, timeout=10) as resp:
raw = json.loads(resp.read())
if isinstance(raw, list):
releases = cast(list[JsonObject], raw)
else:
logger.warning("拉取 release 列表失败,返回 payload 非 JSON 数组: %r", raw)
return ""
except Exception as e: # noqa: BLE001
logger.warning("拉取 release 列表失败,降级为单版 changelog: %s", e)
return ""
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
32ea680 to
7c2f83d
Compare
568d7a4 to
64a91e6
Compare
64a91e6 to
141001d
Compare
Windsland52
left a comment
There was a problem hiding this comment.
Approve — clean, well-scoped PR. No blocking issues.
Verification (ran the project toolchain on PR head 141001d over base 436dc5d):
uv run pyright→ 0 errors, 0 warnings, 0 informationsuv run ruff check .→ all checks passed
The clean pyright run is the key result: removing the project-wide reportMissingTypeArgument = "none" suppression could surface bare-container errors anywhere in the tree. Pyright passing clean confirms every site is covered. Also confirmed the two files whose diff showed no new Any import (aao/measure/api_server.py:14, aao/ui/settings_page.py:12) already import Any.
Annotation-only change; all touched files use from __future__ import annotations, so zero runtime/behavioral impact.
Minor non-blocking notes (pre-existing looseness inherited by the PR, not regressions it introduced):
aao/ui/settings_page.py:390—self._windows: list[Any]could belist[DesktopWindow](underTYPE_CHECKING) for real precision. Was barelistbefore, so the PR just made it pass minimally.aao/measure/api_server.py:37—self._clients: set[Any]similarly loose for a WebSocket client set. Acceptable given the PR's minimal-annotation goal.pyproject.toml— trailing blank line removed (cosmetic, no issue).
Conclusion: the broad suppression is removed, strict mode is retained, and both pyright and ruff pass. Ready to merge.
Summary
typeCheckingMode = strictenabledreportUnknown* = nonebecause MaaFramework callbacks, Qt signals, and JSON settings are dynamic boundaries in this appreportMissingTypeArgument = nonesuppressiondict[str, Any]/list[...]/set[...]annotations at dynamic payload boundaries so bare containers are explicit without introducing project-wide wrapper typesMaaFw, imported asmaa)Validation
uv run pyrightuv run ruff check .uv run pytest